Introduce aarch64-unknown-linux-pauthtest target#155722
Introduce aarch64-unknown-linux-pauthtest target#155722jchlanda wants to merge 10 commits intorust-lang:mainfrom
Conversation
| } | ||
| } | ||
|
|
||
| if sess.target.env == Env::Pauthtest { |
There was a problem hiding this comment.
For a full context I'm going to bring comments from a draft PR into this one:
@asl
Note that pauthtest in clang is an interim thing. How we can enable pauth on, say, bare-metal platform? Or on some downstream platform?
@jchlanda
While there is already code gated by Env in codegen llvm, pauthtest is an outlier, such that all its functionality is behind the environment checks. Handling it earlier in the pipeline would make for a better design, one that decouples target/triple specifics from pauth logic. Maybe Session would be a good candidate to hold that info?
Clang does just that by solving platform/environment specifics on the driver level and later on basing the logic on language flags that are there regardless of the platform. Finally the flags are resolved to a concrete signing schema.
Note that
pauthtestin clang is an interim thing. How we can enable pauth on, say, bare-metal platform? Or on some downstream platform?
Specifically for this bit, I suggested on libc that env could be set to musl since that's what most libraries will care about, and then use the TargetOptions.cfg_abi field for pauthtest. Which means that you can cfg(target_abi = "pauthtest") regardless of whether it's musl or bare metal.
Either way, it could also be encapsulated in an .should_emit_pauth() -> bool function that can do the relevant checks in one place.
There was a problem hiding this comment.
I've created a follow up task to unify handling of pointer authentication features.
There was a problem hiding this comment.
For a full context I'm going to bring comments from a draft PR into this one:
@tgross35
Mentioned this elsewhere but the target should land as no-std first. The initial support should only be the bare minimum to get core building via --target, everything else can come as a followup.
(Fine to keep this PR around as a WIP of course)
There was a problem hiding this comment.
Landing as no-std first makes sense - smaller incremental step are always welcome :)
There was a problem hiding this comment.
@davidtwco, @tgross35 I've extracted away all the library changes from this PR and created a follow up here. I'll update the branches on the new PR once this one lands.
There was a problem hiding this comment.
Feel free to submit it as a draft PR to rust-lang/rust, that makes it easier to reference.
There was a problem hiding this comment.
For a full context I'm going to bring comments from a draft PR into this one:
@tgross35
Could pauth-quicksort-*-driver be combined into one run-make test that builds/runs two different things in main? Since they can probably share some docs or reuse some code, and it saves the per-test overhead.
There was a problem hiding this comment.
@tgross35, not without some serious code juggling; conceptually those two tests perform identical tasks. However, the problem is that pauth-quicksort-c-driver creates a C executable that links against a Rust library. While pauth-quicksort-rust-driver does the opposite, creating a Rust executable that is linked against a C library.
I could put all the sources in one directory and handle compilations from one build script, but am not sure if that would make for a clear, easy to follow test?
|
r? compiler |
|
cc: @davidtwco |
| arch: Arch::AArch64, | ||
|
|
||
| options: TargetOptions { | ||
| env: Env::Pauthtest, |
There was a problem hiding this comment.
I tend to think this should set target_abi = "pauth" instead of target_env = "pauth"? Or maybe an entirely new cfg(target_has_pointer_authentication)?
My motivation is that arm64e Apple targets have pointer authentication as well, but there target_env is used for things like Mac Catalyst too (target_env = "macabi").
There was a problem hiding this comment.
Target triple, aarch64-unknown-linux-pauthtest, mimics exactly what LLVM defines. With pauthtest being llvm::Triple::EnvironmentType.
What I don't like about explicitly using target_abi is that it might set an incorrect expectation. While it is true, that different authentication features, encoded in the signing schema, are not ABI-compatible with one another, pauthtest is not a new ABI, it follows C/C++ language ABI, with pointer authentication features. Also, while LLVM allows for -mabi=pauthtest (i.e.: calng --target=aarch64-linux -mabi=pauthtest) it is then normalized to environment part of the triple effective re-creating aarch64-unknown-linux-pauthtest.
WRT other targets supporting pointer authentication features. I have a follow up task to abstract that away. Clang handles it gracefully, where platform/environment is normalized to a set of language features, that are then used to create a concrete signing schema.
There was a problem hiding this comment.
What I don't like about explicitly using
target_abiis that it might set an incorrect expectation. While it is true, that different authentication features, encoded in the signing schema, are not ABI-compatible with one another,pauthtestis not a new ABI, it follows C/C++ language ABI, with pointer authentication features.
That still feels like it can fit under the definition of "new ABI" IMO? At least if you take the view "if it's not compatible, it's a new ABI".
Anyhow, my primary argument here comes after reading the libc PR, it feels like being able to set target_env = "musl" would make things a lot simpler in most user code?
EDIT: I see now that you've also discussed that with @tgross35.
(If we do that, I'd probably lean towards this target being called aarch64-unknown-linux-muslpauth and it setting target_env = "musl" and target_abi = "pauth", because that would make cc-rs' parsing easier too).
There was a problem hiding this comment.
Currently musl is just an implementation detail and we do not want to set this in the stone. It is just a reference proof-of-concept libc implementation that was done on top of musl in the downstream patch, however, this does not imply that:
- Upstream musl will support pauth anytime soon (and frankly speaking, the real-world implementation should be done a bit differently to ensure e.g. there are no exploitable signing oracles)
- There might be other standard libraries supporting pauth (I am thinking about bare-metal world here mostly)
- Still, some requirements would likely hold for any standard library implementations (e.g. while static link is in theory possible, in reality it would require lots of weird things especially when address discrimination is involved).
To add more on top of it – we're currently discussing ways how we can modify pauthtest approach in Clang/LLVM. So the target triple with environment is an interim solution (this was the motivation of test in the name) that will go away but for now we'd need it in some form :)
There was a problem hiding this comment.
My gut instincts with regards to the target name match @madsmtm's. It's relatively easy for us to change, remove or add new targets with different triples for other standard libraries or environments, so we can make this target as descriptive as possible for the environment is corresponds to.
There was a problem hiding this comment.
From a libs perspective I'd rather have target_env = "musl" as well if that tells us the API we are allowed to use. Otherwise this target is always going to be playing catch up when something gets gated on target_env = "musl" but doesn't account for pauthtest. I also don't know that we need to mirror LLVM if we could do something that's an improvement. (It's fine IMO to not say "musl" in the target triple but still specify that in the env, if that's one of the concerns.)
Perhaps worth a Zulip thread?
There was a problem hiding this comment.
Perhaps worth a Zulip thread?
|
|
||
| pub(crate) fn target() -> Target { | ||
| Target { | ||
| llvm_target: "aarch64-unknown-linux-pauthtest".into(), |
There was a problem hiding this comment.
I feel like the motivation for having "test" in the name is somewhat weak? If the intention is that the target is unstable, we have better ways of enforcing that (such as a check that you're on the nightly channel before using the target).
There was a problem hiding this comment.
pauthtest mimics what LLVM uses: llvm::Triple::EnvironmentType.
There was a problem hiding this comment.
Yeah, I understand that, but I'm not convinced we should carry that forwards? Do you have a link to the original motivation for that name in LLVM?
I guess it depends on how widely you expect to see usage of this target? If there's any point where libraries are going to do target_env = "pauthtest", where it would need to be updated to target_env = "pauth" or smth in the future, then I think it's a bad idea?
There was a problem hiding this comment.
There is an ongoing discussion as to how to move forward with the naming on the LLVM side (see the round table report). My preference would be to stick with pauthtest and wait for LLVM to come up with their new name, which I'll be happy to adopt here.
|
r? madsmtm |
|
|
|
Some changes occurred in compiler/rustc_codegen_gcc This PR modifies cc @jieyouxu This PR modifies If appropriate, please update These commits modify compiler targets.
Some changes occurred in src/tools/compiletest cc @jieyouxu Some changes occurred in cc @Amanieu, @folkertdev, @sayantn Some changes occurred in src/doc/rustc/src/platform-support cc @Noratrieb |
| valid_range: WrappingRange::full(pointer_size), | ||
| }, | ||
| cx.type_ptr_ext(address_space), | ||
| pac_metadata, |
There was a problem hiding this comment.
| pac_metadata, | |
| Some(pac_metadata), |
and just have let pac_metadata not be wrapped in an option?
There was a problem hiding this comment.
I'm not sure, ideally we should only be signing function pointers (addresses, not values), so having the metadata wrapped works as a binary switch. It is true that now, for codegen llvm, we sign everything, but that is a workaround (more info here: #152532).
|
|
||
| fn get_fn_addr(&self, instance: Instance<'tcx>) -> &'ll Value { | ||
| get_fn(self, instance) | ||
| fn get_fn_addr(&self, instance: Instance<'tcx>, pac: Option<PacMetadata>) -> &'ll Value { |
There was a problem hiding this comment.
This function should document the difference between using None versus Some(PacMetadata::default()). When is None allowed, versus when should the default be used?
Co-authored-by: Daniil Kovalev <dkovalev@accesssoftek.com>
`const_ptr_auth` wraps aroudn `LLVMRustConstPtrAuth`, which provides a way to decorate a function pointer in `ConstPtrAuth`.
Allow PAC metadata to be passed to `get_fn_addr` and related API changes.
The set of supported attributes is: function * "aarch64-jump-table-hardening" * "ptrauth-auth-traps" * "ptrauth-calls" * "ptrauth-indirect-gotos" * "ptrauth-returns" module * "ptrauth-elf-got" * "ptrauth-sign-personality"
Also add flag for ELF-GOT signing.
Also: * update tests to force dynamic library when targetting pauthtest * various test fixes * introduce end-to-end tests for pauthtest (in run-make)
|
☔ The latest upstream changes (presumably #149509) made this pull request unmergeable. Please resolve the merge conflicts. |
There was a problem hiding this comment.
Sorry it's taken me a little while, I've read Clang's docs on pointer authentication now, and feel that I have a bit better handle on things now. It's still new to me though, so forgive me if I get things blatantly wrong.
And thanks for splitting out the library parts, that makes this easier to land! Even further, I tend to think you should also split out the new target from the rustc_codegen_* changes - those are harder and need more scrutiny, and also less bikeshed-prone than the target.
Maybe something like Session::c_ptr_auth_abi() that in the beginning returns a simple enum PtrAuthAbi { None }, but which could be expanded with Pauthtest (when the target lands) and perhaps Arm64e in the future?
In any case, it would help me to have more separation between "this adds tooling / setup for doing pointer authentication with LLVM" and "this implements the specific ELF ABI that's in the works", but I get that it may be hard to separate, so don't fret too much if you can't.
| #if LLVM_VERSION_GE(22, 0) | ||
| auto *DeactivationSym = llvm::Constant::getNullValue(PtrTy); | ||
|
|
||
| return wrap(ConstantPtrAuth::get(C, KeyC, DiscC, AddrDiv, DeactivationSym)); |
There was a problem hiding this comment.
Nit: I think I'd prefer for a wrapper like LLVMRustConstPtrAuth to expose the API to its fullest, could you add the DeactivationSym parameter to it, and pass None from the compiler instead?
With the parameter being unused in lower LLVM versions ofc.
| } | ||
| // Only C ABI | ||
| let abi = cx.tcx.fn_sig(def_id).skip_binder().abi(); | ||
| if !matches!(abi, ExternAbi::C { .. }) { |
There was a problem hiding this comment.
extern "C[-unwind]" fn() only? What about other platform ABIs like extern "system"?
There was a problem hiding this comment.
Actually, I tend to think that we should do it for all function pointers, regardless of ABI and DefKind? This would likely help us uncover issues faster, would have the same opsem implications anyhow (extern "C" function pointers aren't special in that sense IIUC), and would mean Rust has the same protections by default as C.
There was a problem hiding this comment.
In C, the arm64e ABI at least doesn't use address diversity (the discriminator is always 0), which is why I'm pretty sure it should be valid for us to do.
Rust probably could use address diversity, since it has stricter rules for what functions are ABI compatible (and it would be nice to have this noted in a comment here), but we don't have to decide on this yet: since Rust doesn't have a stable ABI, we can always change this later.
| OperandValue::Immediate(bx.get_fn_addr(instance, | ||
| Some(PacMetadata::default()), | ||
| )) |
There was a problem hiding this comment.
Nit: formatting seems off here?
| for lib in &mut collector.libs { | ||
| if tcx.sess.target.env == Env::Pauthtest { | ||
| if let NativeLibKind::Static { .. } = lib.kind { | ||
| if !tcx.sess.opts.unstable_opts.ui_testing { |
There was a problem hiding this comment.
Nit: The "proper" way to handle this IIUC would be to make this a lint instead of a warning, and then add it as "-A", "pauthtest_xyz" in src/tools/compiletest/src/runtest.rs? But if it's temporary anyhow, then it's probably fine like this.
View all comments
This target enables Pointer Authentication Code (PAC) support in Rust on AArch64
ELF-based Linux systems. It uses the
aarch64-unknown-linux-pauthtestLLVMtarget and a pointer-authentication-enabled sysroot with a custom musl as a
reference libc implementation. Dynamic linking is required, with a dynamic
linker acting as the ELF interpreter that can resolve pauth relocations and
enforce pointer authentication constraints.
Supported features include:
to LLVM's
-fptrauth-calls)after restoring for non-leaf functions (corresponds to
-fptrauth-returns)(corresponds to
-fptrauth-auth-traps)authentication scheme (corresponds to
-fptrauth-init-finiand-fptrauth-init-fini-address-discrimination)LLVM (corresponds to
-faarch64-jump-table-hardeningand-fptrauth-indirect-gotos)-Z ptrauth-elf-got, off by default)Existing compiler support, such as enabling branch authentication instructions
(i.e.:
-Z branch-protection) provide limited functionality, mainly signingreturn addresses (
pac-ret). The new target goes further by enabling ABI-levelpointer authentication support.
This target does not define a new ABI; it builds on the existing C/C++ language
ABI with pointer authentication support added. However, different authentication
features, encoded in the signing schema, are not ABI-compatible with one
another.
Useful links:
https://clang.llvm.org/docs/PointerAuthentication.html
https://llvm.org/docs/PointerAuth.html
https://github.com/ARM-software/abi-aa/blob/main/pauthabielf64/pauthabielf64.rst
Tier 3 check list
I pledge to do my best maintaining it.
The name chosen for the target is
aarch64-unknown-linux-pauthtestwhichmirrors the LLVM target naming.
There should be no confusion, the name follows naming convention and is
descriptive.
Letters, numbers and dashes only.
The target requires system
clangandlldavailable as well as custom libc(musl based) and sysroot, provided through the build scripts.
There are no license implications.
Understood.
There are no new dependencies or requirements.
The target only relies on open source tools.
No such terms present.
Understood.
Understood.
aarch64-unknown-linux-pauthtest targethas std library support, moreover alllibrarytests pass for the target.Platform support document covers building instructions.
Understood.
Understood.
Understood.
Understood.
It is expected that the target should be able to compile binaries on any systems
that are capable of compiling
aarch64code.